added temporary voice messages #1874

DanielStandfest · 2024-11-10T23:47:27Z

added support for temporary voice messages and handling them just like text messages.

related to #1261

Would be happy to get feedback! :)

Cheers,
Daniel

DanielStandfest · 2024-11-14T00:44:09Z

I am not sure why the localizable change is not passing the test? :/

SystemKeeper · 2024-11-14T07:16:26Z

I am not sure why the localizable change is not passing the test? :/

Interesting, seems to be a sorting issue. I’ll take a look.

SystemKeeper · 2024-11-18T10:59:45Z

I’ll take a look.

So I tried multiple things yesterday and it only happens if you rebase on master (that is what CI does and that's why you can't seem to trigger that change locally in your branch). I have the same issue in #1880. I couldn't fully figure out what's triggering this, even rebasing on old commits did not change that. So for now I'd say we ignore it and take a look later when the PR is merged.

We are going to have a closer look to this and your other PR this week :)

SystemKeeper

I haven't looked through everything, here are some first comments :)

SystemKeeper · 2024-11-18T19:52:47Z

NextcloudTalk/BaseChatViewController.swift


        let referenceId = "temp-\(Date().timeIntervalSince1970 * 1000)"
        temporaryMessage.referenceId = NCUtils.sha1(fromString: referenceId)
        temporaryMessage.internalId = referenceId
        temporaryMessage.isTemporary = true
+        temporaryMessage.isOfflineMessage = true


isOfflineMessage indicates that this message was sent without an internet connection and should be resend, when connection is available again. So this shouldn't be added here.
There's quite some logic involved (including trying to resend in the background). Check out e.g.

talk-ios/NextcloudTalk/NCChatController.m

Line 685 in dd3e20b

- (void)sendChatMessage:(NSString *)message replyTo:(NSInteger)replyTo referenceId:(NSString *)referenceId silently:(BOOL)silently

Thank you for the information, I recognized that the context menu fits better when using the isOfflineMessage = true. That's why I set it to true.
For example the "open in nextcloud" (shouldn't really be an option), "copy" and "create reminder" option dont work out of the box.

I removed it now, should we fix some of the options like "delete" then or hide some of the invalid options?

NextcloudTalk/BaseChatViewController.swift

SystemKeeper · 2024-11-18T20:10:59Z

NextcloudTalk/BaseChatViewController.swift

+                "fileLocalPath": messageParameters
+            ]
+
+            messageParametersDict[parameterId] = fileParameterDict


Why using a UUID here? Should it be the file dictionary? I don't see it used anywhere? (Maybe I missed it, not yet through everything).

Good question, I guess I had a problem with the file parameter not being set when putting the fileParameters in no dictionary.
What's your suggestion?

You can just use "file" as the key here

github-actions · 2024-11-25T02:41:27Z

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

SystemKeeper · 2024-12-13T17:23:31Z

Hey Daniel, left you a message about this PR on our instance :)

SystemKeeper · 2024-12-13T16:20:37Z

NextcloudTalk/BaseChatViewController.swift

@@ -1590,52 +1634,83 @@ import QuickLook
        audioFileName += ".mp3"

        let activeAccount = NCDatabaseManager.sharedInstance().activeAccount()
+        NCAPIController.sharedInstance().setupNCCommunication(for: activeAccount)


Was there a reason to setup the communication here? Because in uniqueNameForFileUploadWithName there's already a setup call and I don't think it's needed before that call.

SystemKeeper · 2024-12-13T16:27:40Z

NextcloudTalk/BaseChatViewController.swift

+            isVoiceMessage: true
+        )
+
+        chatFileController.setFileStatusFrom(temporaryMessage.file())


Since we are not using the chatFileController in this method again (besides moving the file), we don't need the filestatus to be set.

SystemKeeper · 2024-12-13T16:28:29Z

NextcloudTalk/BaseChatViewController.swift

+        chatFileController.setFileStatusFrom(temporaryMessage.file())
+
+        let movedFileToTemporaryDirectory = chatFileController.moveFileToTemporaryDirectory(
+            fromSourcePath: self.recorder!.url.path,


Please use something like guard let recorder = self.recorder else { return } at the beginning of the file, so we don't have to force-unwrap here.

SystemKeeper · 2024-12-13T16:29:15Z

NextcloudTalk/BaseChatViewController.swift

        NCAPIController.sharedInstance().uniqueNameForFileUpload(withName: audioFileName, originalName: true, for: activeAccount, withCompletionBlock: { fileServerURL, fileServerPath, _, _ in
            if let fileServerURL, let fileServerPath, let recorder = self.recorder {
                let talkMetaData: [String: String] = ["messageType": "voice-message"]
-                self.uploadFileAtPath(localPath: recorder.url.path, withFileServerURL: fileServerURL, andFileServerPath: fileServerPath, withMetaData: talkMetaData)
+
+                self.uploadFileAtPath(localPath: recorder.url.path, withFileServerURL: fileServerURL, andFileServerPath: fileServerPath, withMetaData: talkMetaData, temporaryMessage: temporaryMessage)


Suggested change

self.uploadFileAtPath(localPath: recorder.url.path, withFileServerURL: fileServerURL, andFileServerPath: fileServerPath, withMetaData: talkMetaData, temporaryMessage: temporaryMessage)

self.uploadFileAtPath(localPath: destinationFilePath, withFileServerURL: fileServerURL, andFileServerPath: fileServerPath, withMetaData: talkMetaData, temporaryMessage: temporaryMessage)

Otherwise the file is not found on upload (because we moved it before) and we get a 0 byte file.

SystemKeeper · 2024-12-13T16:32:35Z

NextcloudTalk/ChatFileUploader.swift

+                        completion(200, nil)
+                    }
+                }
+            case 404:


In the original code, also case 409 was checked here.

SystemKeeper · 2024-12-13T16:41:56Z

NextcloudTalk/NCChatFileController.h


+- (void)initDownloadDirectoryForAccount:(TalkAccount *)account;
+- (void)setFileStatusFromParameter:(NCMessageFileParameter *)fileParameter;


If I am not mistaken, this is not needed anymore, since we don't call it?!

SystemKeeper · 2024-12-13T16:42:45Z

NextcloudTalk/NCChatFileController.m

+- (void)setFileStatusFromParameter:(NCMessageFileParameter *)fileParameter {
+    _fileStatus = [[NCChatFileStatus alloc] initWithFileId:fileParameter.parameterId fileName:fileParameter.name filePath:fileParameter.path fileLocalPath:fileParameter.fileStatus.fileLocalPath];
+    fileParameter.fileStatus = _fileStatus;


So also not needed?

SystemKeeper · 2024-12-13T16:42:58Z

NextcloudTalk/NCChatFileController.m

+    NSError *error = nil;
+    if ([[NSFileManager defaultManager] moveItemAtPath:sourcePath toPath:destinationPath error:&error]) {
+        NSLog(@"File successfully moved to: %@", destinationPath);
+        _fileStatus.fileLocalPath = destinationPath;


Since the fileStatus is not used, we can remove that line as well

SystemKeeper · 2024-12-13T16:45:40Z

NextcloudTalk/NCChatFileController.m

+- (bool)moveFileToTemporaryDirectoryFromSourcePath:(NSString *)sourcePath destinationPath:(NSString *)destinationPath {
+    TalkAccount *activeAccount = [[NCDatabaseManager sharedInstance] activeAccount];
+
+    [[NCAPIController sharedInstance] setupNCCommunicationForAccount:activeAccount];


We don't need to setup the communication here, as we only move locally.

SystemKeeper · 2024-12-13T17:01:12Z

NextcloudTalk/BaseChatViewController.swift

+                "fileLocalPath": messageParameters
+            ]
+
+            messageParametersDict[parameterId] = fileParameterDict


You can just use "file" as the key here

related to #1261 Signed-off-by: Daniel Standfest <[email protected]>

Signed-off-by: Daniel <[email protected]>

DanielStandfest · 2024-12-16T21:36:53Z

Review implemented.

Ivansss

Thanks a lot for this PR @DanielStandfest :)
Works nicely now and it's a great enhancement!

SystemKeeper · 2024-12-18T15:29:44Z

Thanks again @DanielStandfest ! Merged and will be included in the next release :)
Please remember to rebase instead of creating merge request though ;)

DanielStandfest force-pushed the better-voice-message-handling branch 3 times, most recently from c44beda to dd3e20b Compare November 11, 2024 19:20

DanielStandfest requested review from SystemKeeper and Ivansss November 11, 2024 19:21

SystemKeeper reviewed Nov 18, 2024

View reviewed changes

SystemKeeper mentioned this pull request Nov 19, 2024

Extend mention test to cell rendering #1883

Merged

github-actions bot added the feedback-requested label Nov 25, 2024

DanielStandfest force-pushed the better-voice-message-handling branch from dd3e20b to c7ef13c Compare November 27, 2024 23:15

SystemKeeper reviewed Dec 16, 2024

View reviewed changes

DanielStandfest force-pushed the better-voice-message-handling branch 3 times, most recently from 365e2c7 to b1797e5 Compare December 16, 2024 19:24

added temporary voice messages

35aa953

related to #1261 Signed-off-by: Daniel Standfest <[email protected]>

DanielStandfest force-pushed the better-voice-message-handling branch from c9964ad to 35aa953 Compare December 16, 2024 19:52

Merge branch 'master' into better-voice-message-handling

b740ab8

Signed-off-by: Daniel <[email protected]>

DanielStandfest enabled auto-merge (rebase) December 16, 2024 21:36

DanielStandfest requested a review from SystemKeeper December 16, 2024 21:36

Ivansss approved these changes Dec 17, 2024

View reviewed changes

auto-merge was automatically disabled December 17, 2024 14:58
Rebase failed

Merge branch 'master' into better-voice-message-handling

65eea33

SystemKeeper approved these changes Dec 18, 2024

View reviewed changes

SystemKeeper enabled auto-merge December 18, 2024 14:33

SystemKeeper merged commit b99ba37 into master Dec 18, 2024
11 checks passed

SystemKeeper deleted the better-voice-message-handling branch December 18, 2024 15:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added temporary voice messages #1874

added temporary voice messages #1874

DanielStandfest commented Nov 10, 2024

DanielStandfest commented Nov 14, 2024

SystemKeeper commented Nov 14, 2024

SystemKeeper commented Nov 18, 2024

SystemKeeper left a comment

SystemKeeper Nov 18, 2024

DanielStandfest Nov 27, 2024

SystemKeeper Nov 18, 2024

DanielStandfest Nov 27, 2024

SystemKeeper Dec 13, 2024

github-actions bot commented Nov 25, 2024

SystemKeeper commented Dec 13, 2024

SystemKeeper Dec 13, 2024

SystemKeeper Dec 13, 2024

SystemKeeper Dec 13, 2024

SystemKeeper Dec 13, 2024

SystemKeeper Dec 13, 2024

SystemKeeper Dec 13, 2024

SystemKeeper Dec 13, 2024

SystemKeeper Dec 13, 2024

SystemKeeper Dec 13, 2024

SystemKeeper Dec 13, 2024

DanielStandfest commented Dec 16, 2024

Ivansss left a comment

SystemKeeper commented Dec 18, 2024

	self.uploadFileAtPath(localPath: recorder.url.path, withFileServerURL: fileServerURL, andFileServerPath: fileServerPath, withMetaData: talkMetaData, temporaryMessage: temporaryMessage)
	self.uploadFileAtPath(localPath: destinationFilePath, withFileServerURL: fileServerURL, andFileServerPath: fileServerPath, withMetaData: talkMetaData, temporaryMessage: temporaryMessage)


		- (void)initDownloadDirectoryForAccount:(TalkAccount *)account;
		- (void)setFileStatusFromParameter:(NCMessageFileParameter *)fileParameter;

added temporary voice messages #1874

added temporary voice messages #1874

Conversation

DanielStandfest commented Nov 10, 2024

DanielStandfest commented Nov 14, 2024

SystemKeeper commented Nov 14, 2024

SystemKeeper commented Nov 18, 2024

SystemKeeper left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 25, 2024

SystemKeeper commented Dec 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielStandfest commented Dec 16, 2024

Ivansss left a comment

Choose a reason for hiding this comment

SystemKeeper commented Dec 18, 2024